Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #439 - Add ansible_local support for non-Windows #824

Merged
merged 1 commit into from
Aug 16, 2017

Conversation

swalkinshaw
Copy link
Member

@swalkinshaw swalkinshaw commented Apr 8, 2017

Previously only Windows would use Vagrant's ansible_local provisioner. But it's a much easier experience to get started in development since it skips the Ansible requirement.

This automatically uses the ansible_local provisioner if ansible-playbook does not exist on the host machine.

The env var FORCE_ANSIBLE_LOCAL can be set to force it as well.

Docs: roots/docs#90

@swalkinshaw
Copy link
Member Author

Right now this creates a needless extra synced folder. It probably needs some changes which were done specifically for Windows.

    default: /vagrant => /Users/scott/dev/trellis
    default: /home/vagrant/trellis => /Users/scott/dev/trellis
    default: /home/vagrant/trellis/bin => /Users/scott/dev/trellis/bin

Copy link
Contributor

@fullyint fullyint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This will be really helpful. Everything works great in my tests once that ansible.tags is adjusted. I added an optional change for you to consider for the which(cmd)

Vagrantfile Outdated
if tags = ENV['ANSIBLE_TAGS']
ansible.tags = tags
end
ansible.tags = tags if ENV['ANSIBLE_TAGS']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ansible.tags = ENV['ANSIBLE_TAGS']

Vagrantfile Outdated
end

nil
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an issue that you may figure should be addressed by the user, not by Trellis.

I use pyenv to manage python versions on my machine. Even after uninstalling Ansible, a shim remains at /usr/local/var/pyenv/shims/ansible-playbook, causing an incorrect return value from this which(cmd).

Here's a potential change that resolves it on my machine (dev/null handling from here):

- return exe if File.executable?(exe) && !File.directory?(exe)
+ return system("#{exe} --help", [:out, :err] => File::NULL) if File.executable?(exe) && !File.directory?(exe)

This appeals to me because it tests whether the ansible-playbook command actually works rather than just whether it should work, the latter being assumed based on the presence of the exe. I don't know whether this revised approach would be good practice or acceptable ruby.

It the change above is applied, it returns true, false, or nil, if I recall correctly (instead of exe path). So, maybe the name which() should change to something like cmd_valid(cmd).

@fullyint
Copy link
Contributor

I just tested this latest 3ca2d19 and it works great. Gets around that pyenv issue I had with a previous iteration.

The only other thing I can think of is that the README "Requirements" section could say that Ansible is optional.

@swalkinshaw swalkinshaw force-pushed the ansible-local branch 2 times, most recently from 48ef565 to 78357fd Compare August 16, 2017 03:05
Previously only Windows would use Vagrant's `ansible_local` provisioner.
But it's a much easier experience to get started in development since it
skips the Ansible requirement.

This automatically uses the `ansible_local` provisioner if
`ansible-playbook` does not exist on the host machine.
@swalkinshaw swalkinshaw merged commit 34186ad into master Aug 16, 2017
@swalkinshaw swalkinshaw deleted the ansible-local branch August 16, 2017 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants